Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi: add valueIn parameter to wire.NewTxIn. #1287

Merged
merged 1 commit into from
Jun 16, 2018

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Jun 12, 2018

This adds the valueIn parameter to wire.NewTxIn. Call sites and associated tests have also been updated.

Resolves #1279

@davecgh davecgh added this to the 1.3.0 milestone Jun 12, 2018
rpcserver.go Outdated
@@ -648,8 +648,14 @@ func handleCreateRawTransaction(s *rpcServer, cmd interface{}, closeChan <-chan
"or stake")
}

utxo, err := s.chain.FetchUtxoEntry(txHash)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break the intended behavior of createrawtransaction which is to also allow callers to create transactions for which utxos are not yet known. This information needs to be provided by the caller.

@@ -102,10 +101,10 @@ func TestTx(t *testing.T) {
0xa6, // 65-byte signature
0xac, // OP_CHECKSIG
}
txOut := NewTxOut(txValue, pkScript)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't really accurate. The output amount will almost never match the input amount. The difference is the fee. I don't think they should be tied together like this. It might make sense to do txValue := txIn.ValueIn - 1000 to simulate a fee and then leave all of these references as txValue.

@dnldd dnldd force-pushed the update_nextxin branch 2 times, most recently from f14dc6d to 180d35e Compare June 14, 2018 11:20
@jrick
Copy link
Member

jrick commented Jun 14, 2018

Small suggestion: I would reorder the parameters to make the signature:

NewTxIn(prevOut *OutPoint, valueIn int64, signatureScript []byte) *TxIn

The reason for this is that the prevOut and valueIn parameters will always be required and known by the caller, so they come first, while the signature script may not be immediately known (when creating a new unsigned transaction, for example).

@davecgh
Copy link
Member

davecgh commented Jun 14, 2018

I agree with @jrick regarding the order. I would much prefer the required fields first and optional one last.

This adds the valueIn parameter to wire.NewTxIn. Call sites and associated tests have also been updated.
@dnldd dnldd reopened this Jun 14, 2018
@davecgh davecgh merged commit b105a9e into decred:master Jun 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants